-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce r_wrap_or_clone()
and r_wrap_or_copy()
#1599
base: main
Are you sure you want to change the base?
Conversation
And optimize `NULL` path and clean up a little
int n_kept = 0; | ||
|
||
r_obj* dots = KEEP_N(rlang_dots(env), &n_kept); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved these after the early exit for nm == NULL
because they aren't needed, just for maximal performance and code clarity
nm = KEEP_N(rlang_as_function(nm, env), &n_kept); | ||
nm = KEEP_N(eval_fn_dots(nm, mold, dots, env), &n_kept); | ||
} else { | ||
if (r_length(dots) > 0) { | ||
nm = KEEP_N(eval_fn_dots(c_fn, nm, dots, env), &n_kept); | ||
} | ||
} else if (r_length(dots) > 0) { | ||
nm = KEEP_N(eval_fn_dots(c_fn, nm, dots, env), &n_kept); | ||
} | ||
|
||
if (r_typeof(nm) != R_TYPE_character || r_is_object(nm)) { | ||
nm = KEEP_N(eval_as_character(nm, env), &n_kept); | ||
|
||
if (r_typeof(nm) != R_TYPE_character) { | ||
r_abort("`nm` must be `NULL` or a character vector."); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked this section a little
-
The
nm == function
case didn't callas.character()
on the result of the function, but I felt like it was reasonable to do so since we callas.character()
on the result ofset_names(x, 1, 2)
when...
are combined withc()
, and onset_names(x, 1)
whennm
just isn't a character vector. -
We also had some redundant "if not character, then error" checks below that I've removed in favor of just this single character check.
These are aliases for
R_shallow_duplicate_attr()
andR_duplicate_attr()
respectivelyAdded in 2018, with R 3.6.0, so we can use them without issues in all R versions we support except for 3.5, which we are dropping soon anyways
wch/r-source@bbcf686
I've chosen these function names because they either "wrap" the object into an ALTREP wrapper if possible (i.e. if the type is supported and it is "large enough"), or they call their respective
r_clone/copy()
equivalents.There are 2 places I use these helpers, shown below. They are only useful if you are tweaking attributes and not the main object. Otherwise you should just
r_clone()
.We now use
r_wrap_or_clone_shared()
in the re-encoding helper. If we re-encode an attribute, but not the main object, then we can "wrap" the main object before re-assigning the attributes to it.We now also use
r_wrap_or_clone()
inset_names()
to avoid having to callnames<-
, which is what we were doing before when I thought that was the only way to get access to the ALTREP wrappers. We now only callnames<-
ifx
is an "object" that might have an S3 method.This gives us a little performance boost, but
names<-
was already very fast to call (typically in the 350ns range) so it doesn't look huge. It is relatively large percentage wise though, I guess.I don't show any cases where
x
is an S3 object, that is typically in the 3.5us range because we have to do alength()
dispatch call and anames<-
dispatch call in case there is an S3 method registered, and those cases are completely separate from this PR. We could probably improve on this a little by actually checking to see if there is alength()
ornames<-
method registered in the base/global env and only dispatching if we see one, but I didn't do that here.